Skip to content

build: expand ${ZEPHYR_<module>_MODULE_DIR} in CONF_FILE, OVERLAY_CONFIG #42057

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

build: expand ${ZEPHYR_<module>_MODULE_DIR} in CONF_FILE, OVERLAY_CONFIG #42057

wants to merge 2 commits into from

Conversation

@gregshue
Copy link

@gregshue gregshue commented Jan 21, 2022

Support referencing module directories by name in CONF_FILE, and
OVERLAY_CONFIG so that projects can reference overlay files in arbitrary
modules.

Fixes #41830

Signed-off-by: Gregory Shue [email protected]

@github-actions github-actions bot added area: Build System area: Tests Issues related to a particular existing or missing test labels Jan 21, 2022
tejlmand
tejlmand requested changes Jan 28, 2022
View reviewed changes
Copy link
Contributor

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this proposal.

I see the usefulness in it, but some general thoughts before going into implementation details.

To be consistent between specifying configuration files, then DTC_OVERLAY_FILE should also be updated to have similar behavior.

One concern with this approach is that it will be possible to use any CMake variable, for example -DCONF_FILE=<path>/${FOO}.conf.
If users start to depend on specific variables inside the build system being defined before the CONF_FILE is processed, then we will make it harder for ourselves to refactor our internals, cause suddenly some users might be depending on FOO being defined before CONF_FILE is used. FOO could even be an internal variable.

Of course, some variables could be considered safe, as they will most likely always be available before CONF_FILE is used.
For example: ZEPHYR_<name>_MODULE_DIR and BOARD.
As minimum it should be clearly documented what variables we officially support with this approach.

So please update the documentation to describe this new possibility, together with officially supported variables that can be used.

@gregshue
Copy link
Author

gregshue commented Jan 29, 2022

Thank you. Do you have a doc section in mind for this content?

@tejlmand
Copy link
Contributor

tejlmand commented Jan 31, 2022

Thank you. Do you have a doc section in mind for this content?

As general description, maybe adding it here:
https://docs.zephyrproject.org/latest/application/index.html#application-configuration-directory
for example as continuation of:

Zephyr will use configuration files from the application’s configuration directory except for files with an absolute path provided by the arguments described earlier, for example CONF_FILE, OVERLAY_CONFIG, and DTC_OVERLAY_FILE.

For CONF_FILE I would suggest we also adding this knowledge here:
https://docs.zephyrproject.org/latest/guides/build/kconfig/setting.html#the-initial-configuration

All configuration files will be taken from the application’s configuration directory except for files with an absolute path that are given with the CONF_FILE argument.

with reference to variables that are officially supported with this method.

For devicetree overlays, here:
https://docs.zephyrproject.org/latest/guides/dts/howtos.html#set-devicetree-overlays

@SebastianBoe SebastianBoe removed their request for review January 31, 2022 14:07
@gregshue gregshue force-pushed the 41830-expand-parsing-of-CONF_FILE-OVERLAY_CONFIG branch from 3c67d49 to 0f83faa Compare February 2, 2022 21:33
@gregshue gregshue requested a review from tejlmand February 2, 2022 23:06
tejlmand
tejlmand requested changes Feb 3, 2022
View reviewed changes
Copy link
Contributor

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating the docs.

Some comments, to ensure it become more precise.

@gregshue gregshue force-pushed the 41830-expand-parsing-of-CONF_FILE-OVERLAY_CONFIG branch from 0f83faa to 9ee4392 Compare February 4, 2022 00:16
@gregshue gregshue requested a review from tejlmand February 4, 2022 00:17
tejlmand
tejlmand requested changes Feb 15, 2022
View reviewed changes
Copy link
Contributor

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update.

I took a look at the rendered output, and have a small proposal.

@gregshue gregshue requested a review from tejlmand February 16, 2022 00:59
tejlmand
tejlmand previously approved these changes Feb 16, 2022
View reviewed changes
Copy link
Contributor

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks for this.

Found a stray the to be removed before merging, but approving as everything else is fine.

Gregory Shue and others added 2 commits February 16, 2022 22:37
build: Overlay build vars expand ${ZEPHYR_<module>_MODULE_DIR}
84e809d 
Support referencing module directories by name in CONF_FILE,
OVERLAY_CONFIG, and DTC_OVERLAY_FILE so that projects can reference
overlay files in arbitrary modules.

Fixes zephyrproject-rtos#41830

Signed-off-by: Gregory Shue <[email protected]>
@gregshue @tejlmand
Apply suggestions from code review
99ff090 
Adopted from review suggestions.

Co-authored-by: Torsten Tejlmand Rasmussen <[email protected]>
@gregshue gregshue force-pushed the 41830-expand-parsing-of-CONF_FILE-OVERLAY_CONFIG branch from baca450 to 99ff090 Compare February 17, 2022 12:34
tejlmand
tejlmand approved these changes Mar 9, 2022
View reviewed changes
Copy link
Contributor

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm - re-approved

@tejlmand tejlmand requested a review from de-nordic March 9, 2022 11:08
@github-actions
Copy link

github-actions bot commented May 16, 2022

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label May 16, 2022
@github-actions github-actions bot closed this May 31, 2022
@tejlmand
Copy link
Contributor

tejlmand commented Jun 29, 2022

@gregshue seems this has gone stale.
If you still would like it in, please re-open and rebase, then i'll re-approve

@gregshue
Copy link
Author

gregshue commented Jun 30, 2022

@tejlmand I would still like this in, but I have not recognized any option to reopen the pull request. (I looked at https://github.community/t/re-opening-pull-request/1337/2 for guidance, but the "Reopen pull request" button shown next to "Comment" is not present in my view.)

Guidance?

@dleach02 dleach02 removed the Stale label Jul 1, 2022
@dleach02
Copy link
Member

dleach02 commented Jul 1, 2022

@nashif, are you able to reopen this? The button is greyed out for me

@gregshue
Copy link
Author

gregshue commented Jul 7, 2022

@nashif Are you able to reopen this? I'm not given the option.

@gregshue
Copy link
Author

gregshue commented Jul 11, 2022

@tejlmand @dleach02 @nashif Since we are having trouble re-opening this PR, I created a new one with the same changes (#47638)

@gregshue gregshue mentioned this pull request Jul 15, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tejlmand tejlmand tejlmand approved these changes

@de-nordic de-nordic de-nordic approved these changes

@nashif nashif Awaiting requested review from nashif

Assignees

@tejlmand tejlmand

Labels

area: Build System area: Devicetree area: Documentation area: Tests Issues related to a particular existing or missing test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

CONF_FILE, OVERLAY_CONFIG parsing expands ${ZEPHYR_<whatever>_MODULE_DIR}

4 participants

@gregshue @tejlmand @dleach02 @de-nordic